Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make accessing Debugger registers more ergonomic #997

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Xyene
Copy link
Member

@Xyene Xyene commented Feb 27, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2022

Codecov Report

Merging #997 (d6c7393) into master (485f264) will increase coverage by 2.92%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #997      +/-   ##
==========================================
+ Coverage   81.12%   84.04%   +2.92%     
==========================================
  Files         140      140              
  Lines        4753     4752       -1     
==========================================
+ Hits         3856     3994     +138     
+ Misses        897      758     -139     
Impacted Files Coverage Δ
dmoj/cptbox/isolate.py 89.50% <100.00%> (+37.35%) ⬆️
dmoj/judge.py 52.83% <0.00%> (+1.25%) ⬆️
dmoj/executors/java_executor.py 85.35% <0.00%> (+2.02%) ⬆️
dmoj/cptbox/compiler_isolate.py 55.31% <0.00%> (+6.38%) ⬆️
dmoj/cptbox/tracer.py 76.32% <0.00%> (+18.02%) ⬆️
dmoj/cptbox/handlers.py 100.00% <0.00%> (+25.00%) ⬆️
dmoj/control.py 100.00% <0.00%> (+64.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 485f264...d6c7393. Read the comment docs.

@Xyene Xyene force-pushed the debugger-arg-getitem branch 4 times, most recently from 424f96a to 415fe95 Compare February 27, 2022 20:57
Copy link
Member

@quantum5 quantum5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the idea looks fine...

self.base = base

def __getitem__(self, reg):
return getattr(self.debugger, '%s%d' % (self.base, reg))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you doing this in _cptbox? Maybe you should move this into AdvancedDebugger instead.

@Riolku
Copy link
Contributor

Riolku commented Mar 5, 2022

Agree on Quantum's comment, otherwise looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants